Skip to content

fix(Session): clear session data on destroy in testing environment#10342

Open
gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/session-destroy-testing
Open

fix(Session): clear session data on destroy in testing environment#10342
gr8man wants to merge 2 commits into
codeigniter4:developfrom
gr8man:fix/session-destroy-testing

Conversation

@gr8man

@gr8man gr8man commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Description
In the testing environment, Session::destroy() returned early without calling session_destroy(). While avoiding raw PHP session functions in CLI/testing is expected, doing nothing meant that the mock session data stored in the $_SESSION superglobal remained completely intact. This caused session state to leak between test cases, leading to test pollution.

This PR addresses the issue by resetting the $_SESSION array ($_SESSION = []) when destroy() is called under the testing environment. A unit test has also been added to verify this behavior.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

paulbalandan
paulbalandan previously approved these changes Jun 29, 2026

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but no. session_destroy() does not unset $_SESSION: https://www.php.net/manual/en/function.session-destroy.php

Clearing it only in the testing environment makes Session::destroy() behave differently depending on the environment.

Additionally, CIUnitTestCase::mockSession() already resets $_SESSION during test setup.

@gr8man

gr8man commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I believe we should keep this change for the following reasons:

Immediate Assertions: Without clearing $_SESSION immediately, we cannot easily assert if the logout operation was actually successful within the same test method.

Simulating Real Flow: In production, destroy() is usually followed by a redirect, meaning the next request's $_SESSION is empty. Since CLI tests don't do real redirects, we need to clear it manually to simulate that post-logout state.

State Leakage in Complex Tests: Even with mockSession() in setUp(), if a single test method simulates multiple actions (e.g., logging out User A and then logging in User B), failing to clear the array causes state leakage between those actions.

Mock Usability: A test mock should help us verify application logic (that a session was successfully destroyed) rather than strictly mimicking the annoying limitations of PHP's native session_destroy().

@michalsn

Copy link
Copy Markdown
Member

Immediate Assertions: Without clearing $_SESSION immediately, we cannot easily assert if the logout operation was actually successful within the same test method.

In production, session_destroy() deletes the stored session data, but intentionally does not clear $_SESSION during the current request. A test expecting it to be immediately empty would therefore test behavior that production does not provide.

Simulating Real Flow: In production, destroy() is usually followed by a redirect, meaning the next request's $_SESSION is empty. Since CLI tests don't do real redirects, we need to clear it manually to simulate that post-logout state.

A redirect does not start the next request. It only tells the browser where to go. Session::destroy() should not simulate that next request only when ENVIRONMENT === 'testing'.

State Leakage in Complex Tests: Even with mockSession() in setUp(), if a single test method simulates multiple actions (e.g., logging out User A and then logging in User B), failing to clear the array causes state leakage between those actions.

Logging out one user and logging in another is authentication-level behavior, not merely session destruction. For example, Shield's logout implementation removes authentication data, regenerates the session ID, clears remember-me tokens, resets its internal authentication state, and intentionally keeps the session available for flash messages. An empty $_SESSION is therefore not a reliable way to verify that logout succeeded.

Mock Usability: A test mock should help us verify application logic (that a session was successfully destroyed) rather than strictly mimicking the annoying limitations of PHP's native session_destroy().

Test doubles should avoid external side effects, but their observable behavior should still match production. Otherwise, tests can pass for behavior that does not work in the application. Also, this changes Session itself, not only MockSession.

@paulbalandan paulbalandan dismissed their stale review June 30, 2026 06:21

Ongoing discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants